fix: prevent graph chart infinite loading loop on fullscreen expand (#313) - #322
Conversation
…313) Two root causes addressed: 1. NVL layout timeout — When the graph chart mounts inside a CSS-animated dialog (fullscreen expand), the container starts at ~0 size during the zoom-in-95 animation. NVL's force layout can fail to converge in this state and never fire onLayoutDone, leaving the loading spinner visible indefinitely. Added a safety timeout (800ms) that forces layoutReady if onLayoutDone hasn't fired, then calls fitGraph to re-center. 2. Zustand store conflict — The fullscreen dialog renders a second CardContainer for the same widget, creating two GraphExplorationWrapper instances that both read/write the same graph widget store slot. Added a widgetIdSuffix prop so the fullscreen instance uses a distinct store key (widget.id--fullscreen), preventing re-render cascades between the normal and fullscreen views. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
WalkthroughThe PR adds an 800ms safety timeout to GraphChart's loading state to prevent infinite loading loops when the NVL library's Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…gate Cover the 800ms safety timeout in GraphChart that prevents infinite loading when onLayoutDone never fires, and the widgetIdSuffix prop in CardContainer that prevents graph store conflicts between normal and fullscreen views. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/components/dashboard-container.tsx (1)
354-362:⚠️ Potential issue | 🟠 MajorScope fullscreen suffix to graph widgets only.
At Line 361,
widgetIdSuffix="fullscreen"is passed for every widget type. For parameter widgets, that can change source IDs to...--fullscreen, which won’t match dashboarddata-widget-idvalues and can break “jump to source widget” behavior.Suggested fix
{fullscreenReady ? ( <CardContainer key={`${fullscreenWidget.id}-fullscreen`} widget={fullscreenWidget} refetchInterval={refetchInterval} onNavigateToPage={onNavigateToPage} parameterSourceMap={parameterSourceMap} autoFit - widgetIdSuffix="fullscreen" + widgetIdSuffix={ + fullscreenWidget.chartType === "graph" + ? "fullscreen" + : undefined + } /> ) : (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/dashboard-container.tsx` around lines 354 - 362, The CardContainer is receiving widgetIdSuffix="fullscreen" for every fullscreenWidget which appends --fullscreen to parameter widget IDs and breaks jump-to-source matching; update the render so widgetIdSuffix is only passed for graph widgets (e.g., check fullscreenWidget.type or a helper like isGraphWidget(fullscreenWidget)) and omit widgetIdSuffix for non-graph/parameter widgets, keeping the rest of the props (refetchInterval, onNavigateToPage, parameterSourceMap, autoFit) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/e2e/connections.spec.ts`:
- Around line 190-191: The alert locator is too broad—don't use
page.locator(...).last() because it can pick up alerts from other cards; instead
scope the alert to the clicked card by using the existing card locator (e.g.,
replace page.locator('[role="alert"]').last() with
card.locator('[role="alert"]') or card.locator('role=alert')) and keep the await
expect(...).toBeVisible({ timeout: 5_000 }) call to assert visibility.
---
Outside diff comments:
In `@app/src/components/dashboard-container.tsx`:
- Around line 354-362: The CardContainer is receiving
widgetIdSuffix="fullscreen" for every fullscreenWidget which appends
--fullscreen to parameter widget IDs and breaks jump-to-source matching; update
the render so widgetIdSuffix is only passed for graph widgets (e.g., check
fullscreenWidget.type or a helper like isGraphWidget(fullscreenWidget)) and omit
widgetIdSuffix for non-graph/parameter widgets, keeping the rest of the props
(refetchInterval, onNavigateToPage, parameterSourceMap, autoFit) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e8d44f55-41ba-46bf-b604-16b717e654a3
📒 Files selected for processing (6)
app/e2e/connections.spec.tsapp/src/components/__tests__/card-container.test.tsxapp/src/components/card-container.tsxapp/src/components/dashboard-container.tsxcomponent/src/charts/__tests__/graph-chart.test.tsxcomponent/src/charts/graph-chart.tsx
| const expandedAlert = page.locator('[role="alert"]').last(); | ||
| await expect(expandedAlert).toBeVisible({ timeout: 5_000 }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Playwright, does locator.last() resolve dynamically on each assertion/action, and can it point to a different element after DOM updates?
💡 Result:
Yes, in Playwright, locator.last resolves dynamically on each assertion or action. It re-evaluates the locator query against the current DOM state every time it is used, so it can point to a different element if the DOM updates (such as elements being added, removed, or reordered) between uses. This behavior is core to how all Playwright locators work: they store a query rather than a fixed element reference, ensuring resilience to DOM changes. Official documentation states: "Every time a locator is used for an action, an up-to-date DOM element is located in the page." For example: const locator = page.locator('ul li'); await locator.last.click; // Finds current last
Citations:
- 1: https://playwright.dev/docs/locators
- 2: https://playwright.dev/docs/next/locators
- 3: https://oneuptime.com/blog/post/2026-02-02-playwright-locators/view
- 4: https://stackoverflow.com/questions/75870128/playwright-locator-is-reassigned-after-click
🏁 Script executed:
cat -n app/e2e/connections.spec.ts | sed -n '185,200p'Repository: alfredo1996/neoboard
Length of output: 699
🏁 Script executed:
cat -n app/e2e/connections.spec.ts | sed -n '170,210p'Repository: alfredo1996/neoboard
Length of output: 2072
Scope the alert locator to the clicked card; global .last() is brittle in e2e tests.
On Line 190, page.locator('[role="alert"]').last() re-evaluates dynamically. In e2e tests where multiple cards exist, this can target the wrong alert after DOM changes or when other test cases add alerts to the page. Since card is already scoped on line 180, use it to isolate the alert:
Suggested fix
- const expandedAlert = page.locator('[role="alert"]').last();
+ const expandedAlert = card
+ .locator('[role="alert"]')
+ .filter({ hasText: /refused|ECONNREFUSED|failed|error/i })
+ .first();
await expect(expandedAlert).toBeVisible({ timeout: 5_000 });
// Click again to collapse
await card.click();
- await expect(expandedAlert).not.toBeVisible();
+ await expect(expandedAlert).not.toBeVisible({ timeout: 5_000 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/e2e/connections.spec.ts` around lines 190 - 191, The alert locator is too
broad—don't use page.locator(...).last() because it can pick up alerts from
other cards; instead scope the alert to the clicked card by using the existing
card locator (e.g., replace page.locator('[role="alert"]').last() with
card.locator('[role="alert"]') or card.locator('role=alert')) and keep the await
expect(...).toBeVisible({ timeout: 5_000 }) call to assert visibility.
|
2 participants
Summary
layoutReady=trueif NVLonLayoutDonenever fires (happens when layout starts during dialog animation with degenerate container size)widgetIdSuffixprop to CardContainer to prevent Zustand store conflicts between normal and fullscreen graph instanceswidgetId--fullscreenas store keyCloses #313
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
widgetIdSuffixprop to CardContainer for widget identification flexibilityBug Fixes
Tests